Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Reverse Swap notification #953

Merged
merged 2 commits into from
Jul 7, 2024
Merged

Conversation

dangeross
Copy link
Collaborator

@dangeross dangeross commented Apr 26, 2024

This PR adds background processing of the reverse swap flow using webhooks:

  • Register a reverse swap lockup address for notifications
  • Add process_reverse_swap method to handle processing an individual reverse swap by address
  • Adapt notification plugin handling for the push notification
  • Add ReverseSwapUpdated event

Fixes #924

@dangeross dangeross force-pushed the savage-reverse-swap-notification branch from 5166080 to 76b6cba Compare April 26, 2024 16:17
@dangeross dangeross force-pushed the savage-reverse-swap-notification branch 2 times, most recently from ca76cfd to e63dc2c Compare May 14, 2024 10:07
@dangeross dangeross force-pushed the savage-reverse-swap-notification branch from 0108a93 to 68cec99 Compare May 27, 2024 06:18
@dangeross dangeross force-pushed the savage-reverse-swap-notification branch from 68cec99 to c88c64e Compare June 27, 2024 15:06
@dangeross dangeross force-pushed the savage-reverse-swap-notification branch from c88c64e to 0234597 Compare June 27, 2024 15:10
@dangeross dangeross marked this pull request as ready for review July 3, 2024 06:19
Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! dropped two comments.

}

this.bitcoinAddress?.let {address ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to be able to recognize whether it is a swap or reverse swap without trying both.
If we pass a query param app_data in the webhook url it returns back in the notification payload.
See here where it is grapped from the query param: https://github.com/breez/notify/blob/main/http/router.go#L40
And here how it is populated into the payload: https://github.com/breez/notify/blob/main/breezsdk/init.go#L43
Perhaps we can use it here to identify if it is a swap or reverse swap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought that too, but the redeemSwap first checks there is a swap for that address so it's pretty inexpensive.

What if the application or NDS is already using the app_data param in someway? We would override it and potentially break NDS/app logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good argument. I am ok then to leave this inexpensive check.

libs/sdk-bindings/src/breez_sdk.udl Outdated Show resolved Hide resolved
@dangeross dangeross requested a review from roeierez July 3, 2024 15:23
@dangeross dangeross force-pushed the savage-reverse-swap-notification branch from ec1266c to fb13b8f Compare July 3, 2024 15:45
@@ -194,20 +194,20 @@ class NotificationHelper {
group = LNURL_PAY_WORKGROUP_ID
}
val swapTxConfirmedNotificationChannel = NotificationChannel(
Copy link
Contributor

@erdemyerebasmaz erdemyerebasmaz Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename the notification channel & notification channel group variable as well?

Copy link
Contributor

@erdemyerebasmaz erdemyerebasmaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dangeross dangeross merged commit 8563ff8 into main Jul 7, 2024
9 checks passed
@dangeross dangeross deleted the savage-reverse-swap-notification branch July 7, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add notification for reverse swap chain confirmation
3 participants